replace GetOnlineFeaturesResponse with GetOnlineFeaturesResponseV2 in… #2214
Conversation
… Python Signed-off-by: Tsotne Tabidze <tsotne@tecton.ai>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pyalex, tsotnet The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Tsotne Tabidze <tsotne@tecton.ai>
Codecov Report
@@ Coverage Diff @@
## master #2214 +/- ##
==========================================
+ Coverage 84.98% 85.12% +0.14%
==========================================
Files 105 105
Lines 8450 8453 +3
==========================================
+ Hits 7181 7196 +15
+ Misses 1269 1257 -12
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Signed-off-by: Tsotne Tabidze <tsotne@tecton.ai>
Signed-off-by: Tsotne Tabidze <tsotne@tecton.ai>
| for row_idx, result_row in enumerate(online_features_response.results): | ||
| result_row.values.append(values[row_idx]) | ||
| result_row.statuses.append(FieldStatus.PRESENT) | ||
| result_row.event_timestamps.append(Timestamp()) |
There was a problem hiding this comment.
So python feature server won't provide real event timestamp? and correct status as well
There was a problem hiding this comment.
ah, this is entities. I got it.
But why we put entities in feature vectors?
There was a problem hiding this comment.
Looks like this note got lost during the conflict resolution:
# Note: We temporarily store entities in the same field as features for backwards compatibility.
# Java server uses the same GetOnlineFeaturesResponse but does not return entities right now.
# We need to unify the logic between Python & Java on this.
Basically Python SDK used to return entities as part of the online features response and I don't want to break that with this PR. Although we should have a followup conversation about how to properly resolve this.
| ): | ||
| # Add more feature values to the existing result rows for the request data features | ||
| for feature_name, feature_values in request_data_features.items(): | ||
| proto_values = python_values_to_proto_values( | ||
| feature_values, ValueType.UNKNOWN | ||
| ) | ||
|
|
||
| online_features_response.metadata.feature_names.val.append(feature_name) |
There was a problem hiding this comment.
Why do we put values from request back into response? It seems inconsistent with the fact that we don't put entities in response
There was a problem hiding this comment.
Same reason as for entities
|
/lgtm |
… Python
Signed-off-by: Tsotne Tabidze tsotne@tecton.ai
What this PR does / why we need it: In Java server we started using GetOnlineFeaturesResponseV2 instead of GetOnlineFeaturesResponse as a response proto type. This PR changes Python codebase to also use GetOnlineFeaturesResponseV2. We also get rid of the previous GetOnlineFeaturesResponse definition completely and rename GetOnlineFeaturesResponseV2 to GetOnlineFeaturesResponse. This is a backwards compatible change for the SDK. Namely, we aren't changing the results of
feature_store.get_online_features(...).{to_dict(),to_df()}calls. However, the Python HTTP server's response will be different. This is an example of a response:Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: